fix(client): preserve underlying status code in AutoDetect probe#1530
fix(client): preserve underlying status code in AutoDetect probe#1530MukundaKatta wants to merge 2 commits into
Conversation
27d6da5 to
3aae107
Compare
…llback When the Streamable HTTP probe fails with a non-success status (e.g. 415 Unsupported Media Type) the AutoDetect transport falls back to SSE. If the SSE attempt also fails (e.g. a Streamable-HTTP-only server returns 405 to the GET), the SDK previously surfaced only the SSE error and silently dropped the original Streamable HTTP response, masking the real server diagnostic. Capture the original status and response body up front and, when the SSE fallback also fails, surface the original Streamable HTTP error as the primary HttpRequestException (preserving its status code) with the SSE failure attached as the inner exception. The user now sees the actual server response without losing the fallback detail, and callers can still catch HttpRequestException and read StatusCode. The dual-failure case is also logged at Warning level. Fixes modelcontextprotocol#1526.
…lback fails Regression test for modelcontextprotocol#1526. Triggers the AutoDetect probe via SendMessageAsync (ConnectAsync only constructs the transport) and verifies that the original Streamable HTTP error and its response body are preserved when the SSE fallback also fails. Adds coverage for the surfaced exception shape (HttpRequestException with the SSE failure as inner), cancellation propagation, and the Warning log emitted on the dual-failure path.
3aae107 to
3724379
Compare
| // instead of dropping it on the floor (see https://github.com/modelcontextprotocol/csharp-sdk/issues/1526). | ||
| LogStreamableHttpFailed(_name, response.StatusCode); | ||
|
|
||
| var streamableHttpError = await HttpResponseMessageExtensions.CreateHttpRequestExceptionWithBodyAsync(response, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Reading the body here is correct since it runs before the response is disposed. One thing worth a short comment: when the Streamable HTTP response is application/json but simply isn't a JSON-RPC error, TryReadJsonRpcErrorAsync above has already read the body once. HttpContent buffers after the first read so this second read returns the same buffered content and is safe, but a note would stop a future reader from mistaking it for a stream-consumption bug. For the common non-JSON error responses (415, 405, plain text) there is no double read because TryReadJsonRpcErrorAsync returns early on the content type.
| #if NET | ||
| throw new HttpRequestException(streamableHttpError.Message, sseError, streamableHttpError.StatusCode); | ||
| #else | ||
| throw new HttpRequestException(streamableHttpError.Message, sseError); |
There was a problem hiding this comment.
On net472 this overload can't carry the status code, so callers on that target framework only get the status text in the exception message and not a programmatic StatusCode. That is acceptable given the framework limitation, but it means the preserved-status-code behavior is net5+ only. A one line note here would make it clear this is intentional rather than an oversight.
|
One more thing: the PR description still describes wrapping both failures in an AggregateException. The current code surfaces the original Streamable HTTP HttpRequestException (status code preserved) with the SSE failure as the inner exception instead, so please update the description to match the final approach so reviewers aren't working from the old design. |
|
@MukundaKatta thanks for submitting this PR. I left a few minor comments. Could you please address it or let me know so I can help with it. |
3 participants
Why
HttpTransportMode.AutoDetect(the default) tries Streamable HTTP first and falls back to SSE on any non-success status. When Streamable HTTP returns 415 (e.g. wrongContent-Type, as GitHub Copilot's MCP endpoint does today) and the SSE fallback then fails — common against Streamable-HTTP-only servers that return 405 to the SSEGET— the SDK only surfaces the 405. The original 415 plus its body (the actual server diagnostic) is silently dropped.Result: users debug the wrong protocol. They see "405 Method Not Allowed" against an endpoint they never explicitly tried to
GET, with no hint that the real failure was a content-type mismatch on the Streamable HTTP attempt. Reported in #1526 with a reproducer againsthttps://api.githubcopilot.com/mcp.What
In
AutoDetectingClientSessionTransport:HttpResponseMessageExtensions.EnsureSuccessStatusCodeWithResponseBodyAsync) the response body, building anHttpRequestExceptionvia the existing shared helper.AggregateExceptionwhose first inner is the original Streamable HTTP error and whose second is the SSE failure. The user now sees the real server response without losing the fallback diagnostic.Warning-level log when the SSE fallback fails after Streamable HTTP also failed, so the dual-failure case is visible in logs even when callers swallow the exception.Tested
AutoDetectMode_PreservesOriginalError_WhenStreamableHttpReturns415AndSseFallbackFailsintests/ModelContextProtocol.Tests/Transport/HttpClientTransportAutoDetectTests.cs. Drives the AutoDetect probe viaSendMessageAsync(sinceConnectAsyncfor AutoDetect only constructs the transport — the probe is lazy), simulates the exact 415→405 sequence from the issue withMockHttpHandler, and walks the exception chain to assert the original415status and response body reach the caller.AutoDetectMode_UsesStreamableHttp_WhenServerSupportsIt,AutoDetectMode_FallsBackToSse_WhenStreamableHttpFails) untouched and still pass conceptually — neither relied on a specific exception shape from the dual-failure path.Closes #1526.